Support attaching multiple network interfaces to the same network card #2855

hanwen-cluster · 2024-12-18T20:43:43Z

Prior to this commit, the code had two assumptions:

Single network card instances can have only one network interface
1.1. therefore, when there are more than one network interface, the code made a IMDS call to retrieve device index (network interface index), which is only available on instances with multiple network cards. Having a secondary network interface on single network card instance failed the code and caused instance launch failures.
Each network card can have only one network interface
2.1. therefore, the route table is unique to each network card. Having multiple network interfaces on a network card confused the code and generated wrong route tables.

To fix (1), this commit uses a fallback value 0 when retrieval of device index fails.
To fix (2), this commit names the route tables in a way that is unique to network interface and network card.

FYI:

Network card is the physical card. Network interface is the virtual concept. Each network card can have multiple network interfaces (which in AWS is 1 or 2)
"Network interface" is synonym to "device"

Tests

Manually modified launch template to add secondary network interface to compute nodes. Jobs ran successfully. SSH works on both interfaces.

Checklist

Make sure you are pointing to the right branch.
If you're creating a patch for a branch other than develop add the branch name as prefix in the PR title (e.g. [release-3.6]).
Check all commits' messages are clear, describing what and why vs how.
Make sure to have added unit tests or integration tests to cover the new/modified code.
Check if documentation is impacted by this change.

Please review the guidelines for contributing and Pull Request Instructions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Prior to this commit, the code had two assumptions: 1. Single network card instances can have only one network interface a. therefore, when there are more than one network interface, the code made a IMDS call to retrieve device index (network interface index), which is only available on instances with multiple network cards. Having a secondary network interface on single network card instance failed the code and caused instance launch failures. 2. Each network card can have only one network interface a. therefore, the route table is unique to each network card. Having multiple network interfaces on a network card confused the code and generated wrong route tables. To fix (1), this commit uses a fallback value 0 when retrieval of device index fails. To fix (2), this commit names the route tables in a way that is unique to network interface and network card. FYI: 1. Network card is the physical card. Network interface is the virtual concept. Each network card can have multiple network interfaces (which is AWS is 1 or 2) 2. "Network interface" is synonym to "device" Signed-off-by: Hanwen <[email protected]>

This commit fixes a bug from aws#2855, which made the route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 took priority and failed test_multiple_nics integration test on AL2023. This commit makes the number smaller (meaning higher priority) to fix the issue. e.g. Prior to this commit, the number for table for 1,1 is 1001001. After this commit, the number is 101+10=111. The "+10" is to properly handle table for 0,0, which has number 10. Without "+10", the table would conflict with table 0 from OS. FYI: the number of unwanted default AL2023 rule starts with 10101 Signed-off-by: Hanwen <[email protected]>

This commit fixes a bug from #2855, which made the route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 took priority and failed test_multiple_nics integration test on AL2023. This commit makes the number smaller (meaning higher priority) to fix the issue. e.g. Prior to this commit, the number for table for 1,1 is 1001001. After this commit, the number is 101+10=111. The "+10" is to properly handle table for 0,0, which has number 10. Without "+10", the table would conflict with table 0 from OS. FYI: the number of unwanted default AL2023 rule starts with 10101 Signed-off-by: Hanwen <[email protected]>

aws#2855 made the pcluster route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 ec2-net-utils took priority and failed test_multiple_nics integration test on AL2023. Then, aws#2857 made the number too small, interfering route table configurations from IMDS on AL2. Therefore, this commit tries to imitate the priority prior to these two PRs. This is not the cleanest fix, because it is staying in the lucky priority rand instead of fully resolving the issue (i.e. prevent IMDS and ec2-net-utils from configuring the route tables). However, this commit is the least breaking change. So I propose to go with this commit. Metric number range before the two PRs Network card (0,0): 1000 Network card (0,1): 1000 (which was causing conflicts and the reason for all these PRs) Network card (n,1): 100n (for p5, which has 32 network card, it will be 1000-10031) Metric number range after the first PR: Network card (0,0): 1000000 Network card (0,1): 1000001 (conflict fixed :) ) Network card (n,1): 1000001+n*1000 (for p5, it will be 1000000-1031001) Metric number range after the second PR: Network card (0,0): 10 Network card (0,1): 75 Network card (n,1): 0x(hexadecimal number)n01+10 (for p5, it will be 10-12555. The hexadecimal number was accidentally introduced because bash automatically interpret numbers start with "00" as hexadecimal number) Metric number range after this commit: Network card (0,0): 1000 Network card (0,1): 1001 Network card (n,1): n01+1000 (for p5, it will be 1000-4101) Signed-off-by: Hanwen <[email protected]>

#2855 made the pcluster route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 ec2-net-utils took priority and failed test_multiple_nics integration test on AL2023. Then, #2857 made the number too small, interfering route table configurations from IMDS on AL2. Therefore, this commit tries to imitate the priority prior to these two PRs. This is not the cleanest fix, because it is staying in the lucky priority rand instead of fully resolving the issue (i.e. prevent IMDS and ec2-net-utils from configuring the route tables). However, this commit is the least breaking change. So I propose to go with this commit. Metric number range before the two PRs Network card (0,0): 1000 Network card (0,1): 1000 (which was causing conflicts and the reason for all these PRs) Network card (n,1): 100n (for p5, which has 32 network card, it will be 1000-10031) Metric number range after the first PR: Network card (0,0): 1000000 Network card (0,1): 1000001 (conflict fixed :) ) Network card (n,1): 1000001+n*1000 (for p5, it will be 1000000-1031001) Metric number range after the second PR: Network card (0,0): 10 Network card (0,1): 75 Network card (n,1): 0x(hexadecimal number)n01+10 (for p5, it will be 10-12555. The hexadecimal number was accidentally introduced because bash automatically interpret numbers start with "00" as hexadecimal number) Metric number range after this commit: Network card (0,0): 1000 Network card (0,1): 1001 Network card (n,1): n01+1000 (for p5, it will be 1000-4101) Signed-off-by: Hanwen <[email protected]>

This commit fixes a bug from aws#2855, which made the route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 took priority and failed test_multiple_nics integration test on AL2023. This commit makes the number smaller (meaning higher priority) to fix the issue. e.g. Prior to this commit, the number for table for 1,1 is 1001001. After this commit, the number is 101+10=111. The "+10" is to properly handle table for 0,0, which has number 10. Without "+10", the table would conflict with table 0 from OS. FYI: the number of unwanted default AL2023 rule starts with 10101 Signed-off-by: Hanwen <[email protected]>

aws#2855 made the pcluster route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 ec2-net-utils took priority and failed test_multiple_nics integration test on AL2023. Then, aws#2857 made the number too small, interfering route table configurations from IMDS on AL2. Therefore, this commit tries to imitate the priority prior to these two PRs. This is not the cleanest fix, because it is staying in the lucky priority rand instead of fully resolving the issue (i.e. prevent IMDS and ec2-net-utils from configuring the route tables). However, this commit is the least breaking change. So I propose to go with this commit. Metric number range before the two PRs Network card (0,0): 1000 Network card (0,1): 1000 (which was causing conflicts and the reason for all these PRs) Network card (n,1): 100n (for p5, which has 32 network card, it will be 1000-10031) Metric number range after the first PR: Network card (0,0): 1000000 Network card (0,1): 1000001 (conflict fixed :) ) Network card (n,1): 1000001+n*1000 (for p5, it will be 1000000-1031001) Metric number range after the second PR: Network card (0,0): 10 Network card (0,1): 75 Network card (n,1): 0x(hexadecimal number)n01+10 (for p5, it will be 10-12555. The hexadecimal number was accidentally introduced because bash automatically interpret numbers start with "00" as hexadecimal number) Metric number range after this commit: Network card (0,0): 1000 Network card (0,1): 1001 Network card (n,1): n01+1000 (for p5, it will be 1000-4101) Signed-off-by: Hanwen <[email protected]>

This commit fixes a bug from aws#2855, which made the route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 took priority and failed test_multiple_nics integration test on AL2023. This commit makes the number smaller (meaning higher priority) to fix the issue. e.g. Prior to this commit, the number for table for 1,1 is 1001001. After this commit, the number is 101+10=111. The "+10" is to properly handle table for 0,0, which has number 10. Without "+10", the table would conflict with table 0 from OS. FYI: the number of unwanted default AL2023 rule starts with 10101 Signed-off-by: Hanwen <[email protected]>

aws#2855 made the pcluster route table/metric number larger (meaning lower priority). Thereafter, some unwanted default rules on AL2023 ec2-net-utils took priority and failed test_multiple_nics integration test on AL2023. Then, aws#2857 made the number too small, interfering route table configurations from IMDS on AL2. Therefore, this commit tries to imitate the priority prior to these two PRs. This is not the cleanest fix, because it is staying in the lucky priority rand instead of fully resolving the issue (i.e. prevent IMDS and ec2-net-utils from configuring the route tables). However, this commit is the least breaking change. So I propose to go with this commit. Metric number range before the two PRs Network card (0,0): 1000 Network card (0,1): 1000 (which was causing conflicts and the reason for all these PRs) Network card (n,1): 100n (for p5, which has 32 network card, it will be 1000-10031) Metric number range after the first PR: Network card (0,0): 1000000 Network card (0,1): 1000001 (conflict fixed :) ) Network card (n,1): 1000001+n*1000 (for p5, it will be 1000000-1031001) Metric number range after the second PR: Network card (0,0): 10 Network card (0,1): 75 Network card (n,1): 0x(hexadecimal number)n01+10 (for p5, it will be 10-12555. The hexadecimal number was accidentally introduced because bash automatically interpret numbers start with "00" as hexadecimal number) Metric number range after this commit: Network card (0,0): 1000 Network card (0,1): 1001 Network card (n,1): n01+1000 (for p5, it will be 1000-4101) Signed-off-by: Hanwen <[email protected]>

enrico-usai · 2025-01-23T13:02:39Z

...s-parallelcluster-environment/files/amazon-2023/network_interfaces/configure_nw_interface.sh

 cd "$configuration_directory"

-ROUTE_TABLE=100${DEVICE_NUMBER}
+SUFFIX=$(printf "%03d" $NETWORK_CARD_INDEX)$(printf "%03d" $DEVICE_NUMBER)


it would be good to add an example of the expected output in a comment

enrico-usai · 2025-01-23T13:04:22Z

...s/aws-parallelcluster-environment/files/default/network_interfaces/configure_nw_interface.sh

-echo "Configuring ${DEVICE_NAME} with IP:${DEVICE_IP_ADDRESS} CIDR_PREFIX:${CIDR_PREFIX_LENGTH} NETMASK:${NETMASK} GW:${GW_IP_ADDRESS} ROUTING_TABLE:${ROUTE_TABLE}"
+ROUTE_TABLE="1${SUFFIX}"
+
+echo "Configuring device name: ${DEVICE_NAME} with IP:${DEVICE_IP_ADDRESS} CIDR_PREFIX:${CIDR_PREFIX_LENGTH} NETMASK:${NETMASK} GW:${GW_IP_ADDRESS} ROUTING_TABLE:${ROUTE_TABLE}"


what about adding network card index too? ${NETWORK_CARD_INDEX}

enrico-usai · 2025-01-23T13:06:13Z

.../aws-parallelcluster-environment/files/redhat-8.network_interfaces/configure_nw_interface.sh

-route_table="100${DEVICE_NUMBER}"
-priority="100${DEVICE_NUMBER}"
-metric="100${DEVICE_NUMBER}"
+SUFFIX=$(printf "%03d" $NETWORK_CARD_INDEX)$(printf "%03d" $DEVICE_NUMBER)


To be consistent with existing code we should use lowercase variable names for internal variables, so we should have suffix rather than SUFFIX.

enrico-usai · 2025-01-23T13:13:10Z

cookbooks/aws-parallelcluster-environment/recipes/config/network_interfaces.rb

 return if on_docker?

 def network_card_index(mac, token)
+  # This IMDS call is not available on single NIC instance, therefore fallback to 0


It would be good to link in a comment the public documentation where this behavior is described.

enrico-usai · 2025-01-23T13:17:11Z

...ks/aws-parallelcluster-environment/files/ubuntu/network_interfaces/configure_nw_interface.sh

 FILE="/etc/netplan/${DEVICE_NAME}.yaml"
-ROUTE_TABLE="100${DEVICE_NUMBER}"
+SUFFIX=$(printf "%03d" $NETWORK_CARD_INDEX)$(printf "%03d" $DEVICE_NUMBER)
+ROUTE_TABLE="1${SUFFIX}"


I would have expected kitchen tests to fail in the https://github.com/aws/aws-parallelcluster-cookbook/blob/develop/cookbooks/aws-parallelcluster-environment/test/controls/network_interfaces_spec.rb after this changes.

Could you please run them and double check if we can improve existing coverage?

You can use this file to apply changes on ParallelCluster AMIs from the following PRs: aws#2855 aws#2857 aws#2858 Signed-off-by: Hanwen <[email protected]>

You can use this file to apply changes on ParallelCluster AMIs from the following PRs: #2855 #2857 #2858 Signed-off-by: Hanwen <[email protected]>

You can use this file to apply changes on ParallelCluster AMIs from the following PRs: aws#2855 aws#2857 aws#2858 Signed-off-by: Hanwen <[email protected]>

hanwen-cluster requested review from a team as code owners December 18, 2024 20:43

hanwen-cluster added the skip-changelog-update label Dec 18, 2024

hgreebe approved these changes Dec 19, 2024

View reviewed changes

hanwen-cluster merged commit cb83f4c into aws:develop Dec 19, 2024
33 of 44 checks passed

hanwen-cluster mentioned this pull request Dec 20, 2024

Raise priority of ParallelCluster configured route tables #2857

Merged

hanwen-cluster mentioned this pull request Dec 31, 2024

Configure proper priority for route tables #2858

Merged

enrico-usai reviewed Jan 23, 2025

View reviewed changes

hanwen-cluster mentioned this pull request Jan 24, 2025

Add a patch file to improve resiliency for additional network interface #2859

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support attaching multiple network interfaces to the same network card #2855

Support attaching multiple network interfaces to the same network card #2855

Uh oh!

hanwen-cluster commented Dec 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

enrico-usai Jan 23, 2025

Uh oh!

enrico-usai Jan 23, 2025

Uh oh!

enrico-usai Jan 23, 2025

Uh oh!

enrico-usai Jan 23, 2025

Uh oh!

enrico-usai Jan 23, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Support attaching multiple network interfaces to the same network card #2855

Support attaching multiple network interfaces to the same network card #2855

Uh oh!

Conversation

hanwen-cluster commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Checklist

Uh oh!

Uh oh!

enrico-usai Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

enrico-usai Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

enrico-usai Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

enrico-usai Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

enrico-usai Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hanwen-cluster commented Dec 18, 2024 •

edited

Loading

enrico-usai Jan 23, 2025 •

edited

Loading